Button: remove ui-state-active on button disable. Fixed #9602 - ui-state-active remains after disable #1151
Conversation
…ate-active class remains after disabling a button
|
Added testcase from bug report to use this branch bug_9602. |
|
There's one problem with this approach that I didn't think about earlier. We can't remove |
|
I'll fix that and add some additional test cases to cover those scenarios. |
|
Checked in some additional code to only remove the These two test cases should now show the same behavior for a checkbox
And the test case for issue 9602 |
| @@ -250,7 +250,11 @@ $.widget( "ui.button", { | |||
| this.widget().toggleClass( "ui-state-disabled", !!value ); | |||
| this.element.prop( "disabled", !!value ); | |||
| if ( value ) { | |||
| this.buttonElement.removeClass( "ui-state-focus" ); | |||
| if ( this.buttonElement.is( "input:button, button" ) ) { | |||
There was a problem hiding this comment.
We also support turning <a> elements and even generic elements like <div>s into buttons. It might be better to flip the logic so you explicitly check for radio buttons and checkboxes.
There was a problem hiding this comment.
That is a good point, I did prototype that logic initially but I had a few problems with it. But that is probably a better strategy, I'll take another look at it.
…ate-active remains after disable Refactor unit tests after flipping logic to specifically look for radio and checkbox, also added div, and a elements for unit test.
|
I updated the logic as suggested to specifically check for I updated these two jsfiddle test cases to include more button options.
|
|
👍 |
| $.each( elements, function() { | ||
|
|
||
| var element = $( this ).first(), | ||
| buttonElement = element.is( ":checkbox, :radio" ) ? element.next() : element, |
There was a problem hiding this comment.
:checkbox and :radio are discouraged; use [type=...] selectors.
There was a problem hiding this comment.
Store the result of this check in a variable since you need to know this later too.
|
Please move the test to |
…ate-active class remains after disabling a button Few small formatting changes based upon comments. Moved unit test to button_options.js from button_methods.js.
|
Resolved comments from @scottgonzalez and moved unit test to |
| ]; | ||
|
|
||
| $.each( elements, function() { | ||
|
|
|
A few minor issues, but other than that this looks good to go. Thanks. |
…ate-active class remains after disabling a button use .button( “widget” ) instead of checking for radio or checkbox.
I saw in the style guidelines to use nodeName over tagName. https://contribute.jquery.org/style-guide/js/#dom-node-rules
|
@scottgonzalez there were updates, but no comment, after your last comment - can you review this again? |
| $( "<div></div>" ), | ||
| $( "<input type='checkbox' id='checkbox' checked><label for='checkbox'></label>" ), | ||
| $( "<input type='radio' id='radio' checked><label for='radio'></label>" ) | ||
| ]; |
There was a problem hiding this comment.
Everything is indented too much.
|
Thanks, I squashed everything and landed this. |
Button: remove ui-state-active on button disable. Fixed #9602 - ui-state-active class remains after disabling a button